8387304: [lworld] Review the specs of reflective preview APIs#2578
8387304: [lworld] Review the specs of reflective preview APIs#2578liach wants to merge 6 commits into
Conversation
|
👋 Welcome back liach! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
openjdk
Bot
changed the title
…/lw-reflect-preview-api-0
Webrevs
|
…/lw-reflect-preview-api-0
| * the component type is an interface | ||
| * <li> when preview features are enabled, its {@linkplain | ||
| * <li> when preview features are enabled, its {@link | ||
| * AccessFlag#IDENTITY identity} modifier is always true |
There was a problem hiding this comment.
As it's now a link then maybe "identity" should be "IDENTITY". I realize the other non-link usages are "identity".
| * @since 28 | ||
| */ | ||
| @PreviewFeature(feature = PreviewFeature.Feature.VALUE_OBJECTS) | ||
| @PreviewFeature(feature = PreviewFeature.Feature.VALUE_OBJECTS, reflective = true) |
There was a problem hiding this comment.
Changing the requireIdentity methods to be reflective preview APIs looks okay, as is the update to their docs. My guess is that the libraries that want to get a head start will access the preview APIs with core reflection so they won't care if the methods are just normal preview APIs.
| * If a stack map frame in a {@code class} file has a non-empty list of | ||
| * unset fields, the {@code class} file must declare it uses preview | ||
| * features. The list of unset fields is always empty for a stack map frame | ||
| * declared in a {@code class} file that does not use preview features. |
There was a problem hiding this comment.
It might be a bit more readable to swap the sentences so that it's initially clear that the list is always empty when they class does not use preview features. It may be non-empty when the class uses preview features. Just a suggestion as I had to read the paragraph a few times to digest it.
| * strictly-initialized field. The {@link AccessFlag#STRICT_INIT | ||
| * ACC_STRICT_INIT} flag is considered not set for a field declared in a | ||
| * class or interface that does not use preview features; consequently, | ||
| * this method always returns {@code false} when preview features are disabled. |
There was a problem hiding this comment.
Good, this is an improvement on what we had previously.
| * {@code null} or if the parameter represents a value object when preview | ||
| * features are enabled. Value objects do not exist when preview features | ||
| * are disabled; consequently, this method behaves the same as {@link | ||
| * #nonNull Objects.nonNull} when preview features are disabled. |
There was a problem hiding this comment.
"Value objects do not exist when preview features are disabled". What would you think about just saying that "All objects are identity objects when preview features are disabled"?
| * not have the {@link AccessFlag#IDENTITY ACC_IDENTITY} flag set. | ||
| * The {@code ACC_IDENTITY} flag is considered always set for a class that | ||
| * does not use preview features; consequently, this method always returns | ||
| * {@code false} when preview features are disabled. |
| * <div class="preview-block"> | ||
| * <div class="preview-comment"> | ||
| * When preview features are enabled, if this {@code Class} object | ||
| * represents a class whose {@code class} file does not use preview |
There was a problem hiding this comment.
It might be smoother to start with "When preview features are enabled and this Class object ..."
| * identity} modifier is dependent on whether preview features are | ||
| * enabled, a status not accessible to programs. The preferred way | ||
| * to check for the identity status is to use {@link #isValue() | ||
| * Class.isValue()}. |
There was a problem hiding this comment.
I think it would be better to drop "a status not accessible to programs", it will just anger developers.
Can the second sentence just use "Use the isValue method to test if a value class or identity class"?
| * features are enabled, yet behaves consistently regardless of | ||
| * whether preview features are enabled. | ||
| * {@snippet lang=java : | ||
| * !clazz.isPrimitive() && !clazz.isValue() && !clazz.isInterface() |
There was a problem hiding this comment.
Good idea to include this snippet as it is useful to have.
3 participants
Reflective preview APIs can be used out of preview. Thus it is meaningful for us to note how they behave when preview features are disabled.
I hope this can clarify some common misconceptions and help onboarding to the JEP 401 reflection model.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/2578/head:pull/2578$ git checkout pull/2578Update a local copy of the PR:
$ git checkout pull/2578$ git pull https://git.openjdk.org/valhalla.git pull/2578/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 2578View PR using the GUI difftool:
$ git pr show -t 2578Using diff file
Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/2578.diff
Using Webrev
Link to Webrev Comment